Handle delegated events in bubbling phase wherever possible #40574
Conversation
| @@ -139,7 +139,7 @@ function normalizeParameters(originalTypeEvent, handler, delegationFunction) { | |||
| return [isDelegated, callable, typeEvent] | |||
| } | |||
|
|
|||
| function addHandler(element, originalTypeEvent, handler, delegationFunction, oneOff) { | |||
| function addHandler(element, originalTypeEvent, handler, delegationFunction, options) { | |||
There was a problem hiding this comment.
This is the main change from this PR: whether or not the listener is for the capture phase is now explicit instead of being inferred based on whether this is a delegated handler or not.
I'm very open to changing this API. Suggestions welcome, especially if they mean we can avoid the eslint-disable-next-line max-params later in this file.
js/tests/unit/collapse.spec.js
Outdated
There was a problem hiding this comment.
I believe the change to this test could be made on master and still pass.
| @@ -1358,7 +1358,7 @@ describe('Dropdown', () => { | |||
| btnDropdown.addEventListener('shown.bs.dropdown', () => { | |||
| expect(btnDropdown).toHaveClass('show') | |||
|
|
|||
| const keyup = createEvent('keyup') | |||
| const keyup = createEvent('keyup', { bubbles: true }) | |||
There was a problem hiding this comment.
This and all other createEvent calls I changed are for events that bubble in the browser when they come from keyboard/mouse input. It might be reasonable to change the default for createEvent? I'm not sure. I only changed as many calls as were required to get the tests passing.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Description
This PR changes
EventHandlerto default to listening for events during the bubbling phase (falseis passed as the third argument toaddEventListener), rather than unconditionally handling delegated events in the capture phase.There is only one case (at least that I found while making the test suite pass) where handling events during the capture phase is actually necessary: the implementation of the
data-*attribute API for dropdowns. The tests assert that ifEscis pressed while a dropdown is open, that a) the dropdown closes and b) theEscevent doesn't propagate to other elements. I'm assuming this is there to ensure that pressingEscwhile a dropdown is open in a modal doesn't close both the dropdown and the modal.Motivation & Context
I made this change in response to #36063. I am not necessarily expecting to land this change immediately, though it would be nice. Rather, I'd like to start a discussion with maintainers about changing this, and I felt the best way to do that would be to make the change and get all the tests passing.
To lift the context out of that issue: the current implementation (handling events during the capture phase) makes it impossible to prevent Bootstrap from responding to a click, keyboard press, etc. This can be useful, e.g. if one has implemented a card whose body can collapse by clicking on the header, and the header contains other buttons that should do something besides collapsing/expanding the card's body. An example of this can be found in the linked issue.
If you don't want to make this change, that'd be fine, but #36063 should be closed with rationale for why things are implemented as they are and what workarounds, if any, are suggested.
Type of changes
Honestly, I'm not sure whether to call this a fix, a feature, or a breaking change. If someone is somehow relying on events being handled in the capturing phase, this could in theory affect them, though I'm not sure why someone would do that.
Checklist
npm run lint)Live previews
Related issues
#36063